add gaussian square echo pulse by miamico · Pull Request #9370 · Qiskit/qiskit

miamico · 2023-01-12T23:25:04Z

Summary

Add GaussianSquareEcho symbolic pulse. Similarly to what was done in #9329

Details and comments

The GaussianSquareEcho pulse is comprised of two tones driving the target qubit during the cross-resonance gate. The echoed pulse is a rotary tone while the Gaussian square pulse overlaid on it is a cancellation tone. The pulse shape was introduced in this paper and an example visualization can be found at the bottom panel of figure 7a of this paper (the red and orange curve of the DCX plot).

Open to suggestions for which tests to come up for it.

CLAassistant · 2023-01-12T23:25:09Z

All committers have signed the CLA.

CLAassistant · 2023-01-12T23:25:10Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

qiskit-bot · 2023-01-12T23:25:13Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@nkanazawa1989

TsafrirA

Hi @miamico ,
Looks like an interesting addition. While not strictly causing any breakdowns right now, the use of ScalableSymbolicPulse goes against the definition of the class - and might cause problems down the road.
I think there are two options here:

Use a general purpose SymbolicPulse instead of ScalableSymbolicPulse.
Force the envelope form of ScalableSymbolicPulse on your envelope.

The latter could be done by defining rel_active_amp instead of active_amp and defining the total envelope as: amp*(envelope_expr + rel_active_amp*envelope_expr_xy) (where amp and active_amp are removed from the original expressions).

@nkanazawa1989 could help out here.

TsafrirA · 2023-01-26T15:29:23Z

@miamico
In addition to the stuff we discussed before, you need to make some Lynt corrections, so the CI tests will pass. Check out this page.

You also need to add release notes - check out here, and draw inspiration from @wshanks PR #9329 .

It will also be healthy to add some tests to make sure this pulse functions as expected (Doing this would have caught the incorrect instantiation we had, for example). Check out this test and maybe add to the QPY test here.

wshanks · 2023-01-26T18:44:13Z

Thanks, @miamico! This is looking good. I put a few comments in. Besides what Tsafrir mentioned, please fill out the CLA form here (linked from the CLAassistant comment above).

coveralls · 2023-01-31T20:06:21Z

Pull Request Test Coverage Report for Build 4823142933

35 of 36 (97.22%) changed or added relevant lines in 2 files are covered.
18 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.05%) to 85.85%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/pulse/library/symbolic_pulses.py	34	35	97.14%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	2	91.14%
crates/accelerate/src/sabre_swap/layer.rs	4	97.32%
crates/qasm2/src/parse.rs	12	96.65%

Totals
Change from base Build 4822671868:	-0.05%
Covered Lines:	70791
Relevant Lines:	82459

💛 - Coveralls

nkanazawa1989

Thanks @miamico this looks almost good to go. IIRC you are doing this because some deployed devices will report DCX pulse in this form. To let them use parametric form, you need to register this pulse name to this enum.
https://github.com/Qiskit/qiskit-terra/blob/d814ad43c81e2b285ea91db4f4bba2234bc2b7db/qiskit/qobj/converters/pulse_instruction.py#L41-L46

Also could you please add unittest in this class?
https://github.com/Qiskit/qiskit-terra/blob/d814ad43c81e2b285ea91db4f4bba2234bc2b7db/test/python/pulse/test_pulse_lib.py#L122
You can find several tests with GaussianSquareDrag for example.

I will approve and merge this PR with these updates.

nkanazawa1989 · 2023-04-18T16:36:03Z

This PR fails in the docs build. See the following Sphinx guide for the literal block formatting.
https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#literal-blocks

(the error message is not really friendly)

wshanks · 2023-04-19T16:17:22Z

In case it is helpful, here is what I see when I build the docs locally:

wshanks

Here is some feedback on the doc string. Otherwise this PR looks good to me (except maybe the suggestion to rename amp and angle).

wshanks · 2023-04-19T16:24:11Z

+               docstring for more details.
+        width: The duration of the embedded square pulse.
+        angle: The angle in radians of the complex phase factor uniformly
+            scaling the pulse. Default value 0.


It needs to be clarified that this angle affects only the echoed part of the pulse, not the entire pulse. I would change to amp and angle to echo_amp and echo_angle to put the two parts on equal footing, but I don't feel too strongly about it.

I have tried to be more specific in the docstring on what refers to what. I'd like to avoid changing variable names as amp and angle are the standard variable names used for other pulses (and one could define a GaussianSquareEcho just by defining these two).

miamico · 2023-04-22T23:47:24Z

All checks are passing now

wshanks

This looks good to me now. I just had one suggestion.

Co-authored-by: Will Shanks <wshaos@posteo.net>

wshanks

This looks good to me, but I will leave it to @TsafrirA and @nkanazawa1989 to approve and merge!

miamico · 2023-04-27T18:23:06Z

great! thanks :)

wshanks · 2023-05-01T15:45:35Z

@miamico Sorry about this extra work, but we decided to move forward with the deprecation of the symbolic pulse classes (which are in pending status now) in favor of functions. Could you change all of the GaussianSquareEcho usage to gaussian_square_echo so that this pulse does not need to transition its name later?

wshanks

I found two tiny things to change. If those look right to you, we can merge.

Co-authored-by: Will Shanks <wshaos@posteo.net>

….yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

miamico · 2023-05-19T21:19:52Z

I found two tiny things to change. If those look right to you, we can merge.

Yes totally! I've committed those suggestions :)

nkanazawa1989

Thanks. This looks good to go. I'll add this to merge queue.

* add gaussian square echo pulse * address some PR comments * fixed parameters of symbolicpulse * adding math description and reference * change variable names * fix lint * remove width_echo parameter * fix lint * add release notes * fix conflicts * address some PR comments * fixed parameters of symbolicpulse * adding math description and reference * change variable names * fix lint * remove width_echo parameter * fix lint * add release notes * Update first paragraph Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update amp param to float Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update angle def param to float Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update amp constrain Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * drafting tests * update docstring * finish up test * fix missing pulses from init * addding Cos * missing sin * fixed tests * black formatting * break string * fixing strings * reformatting * fix lint * fixing last things * fix tests * fix black * fix another test * fix amp validation * fixing docstring * Update qiskit/pulse/library/symbolic_pulses.py Co-authored-by: Will Shanks <wshaos@posteo.net> * rename to gaussian_square_echo * fix lint * Update qiskit/qobj/converters/pulse_instruction.py Co-authored-by: Will Shanks <wshaos@posteo.net> * Update releasenotes/notes/gaussian-square-echo-pulse-84306f1a02e2bb28.yaml Co-authored-by: Will Shanks <wshaos@posteo.net> --------- Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> Co-authored-by: Will Shanks <wshaos@posteo.net>

add gaussian square echo pulse

cd37e22

miamico requested review from a team, eggerdj and wshanks as code owners January 12, 2023 23:25

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jan 12, 2023

mtreinish added this to the 0.24.0 milestone Jan 13, 2023

wshanks removed the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jan 13, 2023

TsafrirA reviewed Jan 15, 2023

View reviewed changes

ajavadia added the mod: pulse Related to the Pulse module label Jan 20, 2023

miamico added 2 commits January 24, 2023 15:20

address some PR comments

859c451

fixed parameters of symbolicpulse

760aa7c

wshanks reviewed Jan 26, 2023

View reviewed changes

Comment thread qiskit/pulse/library/symbolic_pulses.py Outdated

wshanks reviewed Jan 26, 2023

View reviewed changes

Comment thread qiskit/pulse/library/symbolic_pulses.py Outdated

nkanazawa1989 mentioned this pull request Jan 27, 2023

include ecr native gate for pulse scaling #9397

Merged

miamico added 4 commits January 31, 2023 10:12

adding math description and reference

fca8fd6

change variable names

2895969

fix lint

755d16e

remove width_echo parameter

6f73a4c

miamico added 2 commits January 31, 2023 13:35

fix lint

e1c731e

add release notes

b083550

mtreinish assigned nkanazawa1989 Apr 11, 2023

nkanazawa1989 reviewed Apr 12, 2023

View reviewed changes

nkanazawa1989 modified the milestones: 0.24.0, 0.25.0 Apr 12, 2023

wshanks reviewed Apr 19, 2023

View reviewed changes

miamico and others added 3 commits April 22, 2023 14:29

fixing docstring

257fd43

Merge branch 'main' of github.com:miamico/miamico-gse

75183f2

Merge branch 'main' into main

97a1e5f

wshanks reviewed Apr 27, 2023

View reviewed changes

Comment thread qiskit/pulse/library/symbolic_pulses.py Outdated

Update qiskit/pulse/library/symbolic_pulses.py

bb744bd

Co-authored-by: Will Shanks <wshaos@posteo.net>

wshanks previously approved these changes Apr 27, 2023

View reviewed changes

Merge branch 'main' into main

8be636a

miamico and others added 3 commits May 2, 2023 13:57

Merge branch 'main' into main

769b145

rename to gaussian_square_echo

88f045f

Merge branch 'main' of github.com:miamico/miamico-gse

be39a8f

miamico dismissed wshanks’s stale review via be39a8f May 2, 2023 20:04

fix lint

efb3b07

miamico requested review from TsafrirA, nkanazawa1989 and wshanks May 19, 2023 20:03

wshanks reviewed May 19, 2023

View reviewed changes

Comment thread qiskit/qobj/converters/pulse_instruction.py Outdated

Comment thread releasenotes/notes/gaussian-square-echo-pulse-84306f1a02e2bb28.yaml Outdated

miamico and others added 2 commits May 19, 2023 14:19

Update qiskit/qobj/converters/pulse_instruction.py

41647e1

Co-authored-by: Will Shanks <wshaos@posteo.net>

Update releasenotes/notes/gaussian-square-echo-pulse-84306f1a02e2bb28…

4328ce2

….yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

nkanazawa1989 approved these changes May 20, 2023

View reviewed changes

nkanazawa1989 added this pull request to the merge queue May 20, 2023

Merged via the queue into Qiskit:main with commit bedecbd May 20, 2023

Conversation

miamico commented Jan 12, 2023

Summary

Details and comments

Uh oh!

CLAassistant commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Jan 12, 2023

Uh oh!

qiskit-bot commented Jan 12, 2023

Uh oh!

TsafrirA left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TsafrirA commented Jan 26, 2023

Uh oh!

Uh oh!

Uh oh!

wshanks commented Jan 26, 2023

Uh oh!

coveralls commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 4823142933

💛 - Coveralls

Uh oh!

nkanazawa1989 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkanazawa1989 commented Apr 18, 2023

Uh oh!

wshanks commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wshanks left a comment

Choose a reason for hiding this comment

Uh oh!

wshanks Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

miamico Apr 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miamico commented Apr 22, 2023

Uh oh!

wshanks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wshanks left a comment

Choose a reason for hiding this comment

Uh oh!

miamico commented Apr 27, 2023

Uh oh!

wshanks commented May 1, 2023

Uh oh!

wshanks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miamico commented May 19, 2023

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

CLAassistant commented Jan 12, 2023 •

edited

Loading

TsafrirA left a comment •

edited

Loading

coveralls commented Jan 31, 2023 •

edited

Loading

nkanazawa1989 left a comment •

edited

Loading

wshanks commented Apr 19, 2023 •

edited

Loading